cmuxTests/SessionPersistenceTests.swift (1)

1196-1203: Consider adding a test that verifies destructive commands aren't matched by default patterns.

The current test verifies that literal patterns like "rm *" aren't in defaultAllowlistPatterns, but doesn't verify that isCommandAllowed("rm -rf /", rawAllowlist: defaultRawAllowlist) returns false. If a future change added an overly broad pattern, this test wouldn't catch it.

💡 Suggested additional test

func testDefaultAllowlistDoesNotMatchDestructiveCommands() {
    let defaultRaw = SessionRestoreCommandSettings.defaultAllowlistPatterns.joined(separator: "\n")
    XCTAssertFalse(SessionRestoreCommandSettings.isCommandAllowed("rm -rf /", rawAllowlist: defaultRaw))
    XCTAssertFalse(SessionRestoreCommandSettings.isCommandAllowed("sudo rm -rf /", rawAllowlist: defaultRaw))
    XCTAssertFalse(SessionRestoreCommandSettings.isCommandAllowed("git push --force", rawAllowlist: defaultRaw))
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/SessionPersistenceTests.swift` around lines 1196 - 1203, Add a new
unit test that asserts the default allowlist patterns do not match destructive
commands: use SessionRestoreCommandSettings.defaultAllowlistPatterns joined into
a rawAllowlist string and call
SessionRestoreCommandSettings.isCommandAllowed(...) with inputs like "rm -rf /",
"sudo rm -rf /", and "git push --force", asserting each returns false; place
this alongside the existing testDefaultAllowlistExcludesDestructiveCommands
(e.g., name it testDefaultAllowlistDoesNotMatchDestructiveCommands) so the suite
covers both absence of literal patterns and actual matching behaviour.

Sources/SessionPersistence.swift (1)

546-546: Empty allowlist falls back to defaults instead of disabling all restores.

If a user intentionally clears the allowlist (e.g., sets it to whitespace or all comments) to disable command restoration entirely, the current logic falls back to defaultAllowlistPatterns. This may not match user intent.

Consider whether an empty parsed result should mean "allow nothing" (return []) rather than "use defaults". If you want to preserve the current behavior, document it clearly in part 2's Settings UI.

♻️ Alternative: respect empty allowlist as "allow nothing"

     static func normalizedAllowlistPatterns(rawValue: String?) -> [String] {
         let source: String
         if let rawValue, !rawValue.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
             source = rawValue
         } else {
             source = defaultAllowlistText
         }
         let parsed = parsePatterns(from: source)
-        return parsed.isEmpty ? defaultAllowlistPatterns : parsed
+        // If user provided a rawValue but it parsed to empty, respect that as "allow nothing"
+        if rawValue != nil && parsed.isEmpty {
+            return []
+        }
+        return parsed.isEmpty ? defaultAllowlistPatterns : parsed
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/SessionPersistence.swift` at line 546, The current return uses
defaultAllowlistPatterns when parsed is empty which overrides an intentional
empty allowlist; change the logic in SessionPersistence (the code returning
parsed.isEmpty ? defaultAllowlistPatterns : parsed) so that an explicitly-empty
parsed result is respected (return parsed or return [] when parsed.isEmpty) and
only fall back to defaultAllowlistPatterns when parsing failed/parsed is nil;
update any related docstring/comment in the function and adjust tests/UI for the
Settings allowlist behavior to reflect that an empty allowlist disables
restores.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Sources/AppDelegate.swift (1)

3712-3713: ⚠️ Potential issue | 🟠 Major

group.wait() keeps snapshot saves UI-blocking.

saveSessionSnapshot still runs on @MainActor, and refreshForegroundProcessCacheSync() waits synchronously for SessionForegroundProcessCache.refresh(...), which is itself a blocking per-TTY ps loop. Autosave, resign-active, and quit-time saves can still stall the app until the full scan finishes. Please either refresh ahead of time and consume the last completed cache here, or move the refresh+save pipeline off the main actor.

Also applies to: 3988-4000

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 3712 - 3713, The call to
refreshForegroundProcessCacheSync() blocks the main thread because
saveSessionSnapshot remains on `@MainActor` while
SessionForegroundProcessCache.refresh(...) does a per-TTY ps loop; change the
flow so the snapshot save does not wait for that blocking refresh: either (A)
pre-run the refresh on a background queue and have
refreshForegroundProcessCacheSync() read and return the last completed cache
(consume cache snapshot instead of running refresh synchronously) or (B) move
the refresh+save pipeline off the main actor entirely by performing
SessionForegroundProcessCache.refresh(...) on a background Task/DispatchQueue
and then call saveSessionSnapshot only after that background work completes (or
call a non-@MainActor helper that collects data then dispatches the minimal
UI-bound work back to `@MainActor`). Update refreshForegroundProcessCacheSync,
SessionForegroundProcessCache.refresh, and saveSessionSnapshot call sites to use
the chosen approach so the UI is not blocked.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Sources/SessionPersistence.swift (1)

812-819: Consider extracting shell names to a static constant.

The inline list works correctly, but extracting it as a static property would improve readability and make future additions easier to manage.

♻️ Optional refactor

 enum SessionForegroundProcessDetector {
     private static let psPath = "/bin/ps"
+    
+    /// Shell executables to skip when detecting foreground commands.
+    private static let shellExecutables: Set<String> = [
+        "zsh", "bash", "sh", "fish", "tcsh", "ksh", "dash",  // POSIX-ish shells
+        "csh",                                               // C shell
+        "pwsh", "powershell",                                // PowerShell
+        "nu", "nushell",                                     // Nushell
+        "elvish", "xonsh", "oil", "osh",                     // Alternative shells
+        "rc", "es",                                          // Plan 9 shells
+    ]
 
     // ... in detect(forTTY:) ...
-        let shellNames = [
-            "zsh", "bash", "sh", "fish", "tcsh", "ksh", "dash",
-            // ...
-        ]
-        if shellNames.contains(foreground.executableName) {
+        if shellExecutables.contains(foreground.executableName) {
             return nil
         }

Using a Set also gives O(1) lookup instead of O(n), though with ~15 elements the difference is negligible.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/SessionPersistence.swift` around lines 812 - 819, Extract the inline
shellNames array into a shared static constant on the SessionPersistence type
(e.g. static let knownShells: Set<String> = [...]) and replace the local let
shellNames usage with SessionPersistence.knownShells (or Self.knownShells inside
instance methods); convert the collection to a Set for O(1) lookup and keep the
same elements (zsh, bash, sh, fish, tcsh, ksh, dash, csh, pwsh, powershell, nu,
nushell, elvish, xonsh, oil, osh, rc, es) to preserve behavior and simplify
future maintenance.

Sources/Workspace.swift (1)

669-675: Reuse setPanelRestoreCommand here too.

This reimplements the same trim/allowlist policy that already lives in setPanelRestoreCommand, so the restore path can drift from the socket and reattach paths.

♻️ Suggested simplification

-            // Persist explicit restoreCommand if allowed (blocked commands don't become sticky)
-            if let restoreCommand = snapshot.terminal?.restoreCommand, !restoreCommand.isEmpty {
-                let trimmed = restoreCommand.trimmingCharacters(in: .whitespacesAndNewlines)
-                if SessionRestoreCommandSettings.isCommandAllowed(trimmed) {
-                    setPanelRestoreCommand(panelId: terminalPanel.id, command: trimmed)
-                }
-            }
+            // Persist explicit restoreCommand if allowed (blocked commands don't become sticky)
+            setPanelRestoreCommand(panelId: terminalPanel.id, command: snapshot.terminal?.restoreCommand)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 669 - 675, The code duplicates the
trim/allowlist logic already implemented in setPanelRestoreCommand; remove the
manual trimming and SessionRestoreCommandSettings check and simply call
setPanelRestoreCommand(panelId: terminalPanel.id, command: restoreCommand) when
snapshot.terminal?.restoreCommand is non-nil/non-empty so the same normalization
and allowlist enforcement is used consistently for restore, socket, and reattach
paths.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Sources/Workspace.swift (1)

237-241: Skip remote-backed terminals from the cache refresh set.

sessionPanelSnapshot(...) now ignores cached foreground detections for remote-backed terminals, so collecting their TTYs here only adds extra ps work during the pre-snapshot refresh. Reusing the same remote-backed predicate here would trim unnecessary quit-time work.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 237 - 241, In allTerminalTTYNames(),
filter out terminal panels that are remote-backed the same way
sessionPanelSnapshot(...) does so we don't include their TTYs in
surfaceTTYNames; update the compactMap closure (which currently checks "panel is
TerminalPanel") to also check the existing remote-backed predicate (reuse the
same method/property used by sessionPanelSnapshot, e.g. panel.isRemoteBacked or
the project's remote-backed check) and return nil for remote-backed terminals so
only local terminal TTYs from surfaceTTYNames[panelId] are collected.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Sources/Workspace.swift (1)

749-753: ⚠️ Potential issue | 🟠 Major

Skip restoring listeningPorts for remote-backed panels too.

Line 751 correctly stops rehydrating dead-session TTYs, but Line 749 still restores snapshot.listeningPorts unconditionally. Please reuse the same remote-backed predicate here; otherwise a legacy or tampered snapshot can repopulate remote-only ports after restore.

🛡️ Suggested fix

-        surfaceListeningPorts[panelId] = Array(Set(snapshot.listeningPorts)).sorted()
+        let isRemoteBackedTerminal =
+            activeRemoteTerminalSurfaceIds.contains(panelId) ||
+            transferredRemoteCleanupConfigurationsByPanelId[panelId] != nil
+        surfaceListeningPorts[panelId] = isRemoteBackedTerminal
+            ? []
+            : Array(Set(snapshot.listeningPorts)).sorted()

Based on learnings: remote-detected listening ports must never be persisted, and applySessionPanelMetadata must not restore any remote listening ports during session rehydration.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 749 - 753, In
applySessionPanelMetadata, don't unconditionally restore snapshot.listeningPorts
into surfaceListeningPorts; wrap the assignment of
surfaceListeningPorts[panelId] = Array(Set(snapshot.listeningPorts)).sorted() in
the same "remote-backed" check used to skip restoring ttyName so that
remote-backed panels never get listeningPorts reapplied from a snapshot—only
restore snapshot.listeningPorts for non-remote-backed panels.

Sources/SessionPersistence.swift (1)

754-770: ⚠️ Potential issue | 🟠 Major

refreshSync still blocks the quit path.

Sources/AppDelegate.swift calls the synchronous variant from applicationShouldTerminate(...) before saving the session. That means quit still waits on every ps + sysctl pass here, so shutdown can stall under load or with many TTYs. I’d avoid the sync refresh on termination and fall back to the last async cache snapshot, or add a bounded timeout/fallback.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/SessionPersistence.swift` around lines 754 - 770, The synchronous
refreshSync(ttyNames:) currently blocks shutdown; change the termination path to
avoid calling it: in applicationShouldTerminate(...) stop invoking refreshSync
and instead read the most recent async snapshot from cache (protected by lock)
or implement a bounded-sync variant that dispatches the same work onto queue
asynchronously and waits with a timeout (e.g. DispatchSemaphore/DispatchGroup
wait(timeout:)), then falls back to the existing cache if the wait times out.
Update references to queue, cache, and refreshSync to use the new non-blocking
fallback so quitting does not stall on long ps/sysctl calls.

Sources/AppDelegate.swift (1)

3994-4005: Deduplicate TTY names before refresh to avoid redundant scans.

Line 3994 and Line 4002 can pass duplicate TTYs if contexts overlap; deduping reduces unnecessary ps work with no behavior change.

♻️ Proposed refactor

 private func refreshForegroundProcessCacheAsync() {
-    let allTTYNames = mainWindowContexts.values.flatMap { context in
+    let allTTYNames = Set(mainWindowContexts.values.flatMap { context in
         context.tabManager.allTerminalTTYNames()
-    }
-    SessionForegroundProcessCache.shared.refresh(ttyNames: Array(allTTYNames))
+    })
+    SessionForegroundProcessCache.shared.refresh(ttyNames: Array(allTTYNames))
 }
 
 private func refreshForegroundProcessCacheSync() {
-    let allTTYNames = mainWindowContexts.values.flatMap { context in
+    let allTTYNames = Set(mainWindowContexts.values.flatMap { context in
         context.tabManager.allTerminalTTYNames()
-    }
-    SessionForegroundProcessCache.shared.refreshSync(ttyNames: Array(allTTYNames))
+    })
+    SessionForegroundProcessCache.shared.refreshSync(ttyNames: Array(allTTYNames))
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 3994 - 4005, The code gathers TTY
names with mainWindowContexts.values.flatMap { context in
context.tabManager.allTerminalTTYNames() } and then calls
SessionForegroundProcessCache.shared.refresh(...) and refreshSync(...);
deduplicate the collected TTY names before calling refresh/refreshSync (e.g.,
convert to a Set then back to Array) so duplicate TTYs from overlapping contexts
are removed and redundant ps scanning is avoided. Ensure you update both call
sites that use allTTYNames and keep the same types expected by refresh and
refreshSync.

cmuxTests/SessionPersistenceTests.swift (1)

1219-1305: Consider table-driven denylist cases to reduce repetition.

Most denylist tests follow the same arrange/assert shape; a parameterized helper would keep the same coverage with lower maintenance overhead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/SessionPersistenceTests.swift` around lines 1219 - 1305, Refactor
the repetitive denylist tests by introducing a table-driven helper (e.g., a
private helper method in the test file) that calls
SessionRestoreCommandSettings.isCommandAllowed for a given rawAllowlist and an
array of commands and asserts they are all false; replace repeated patterns in
testDenylistBlocksDestructiveCommandsEvenIfInAllowlist,
testDenylistIsCaseInsensitive, testDenylistBlocksExactCommands,
testDenylistBlocksSystemCommands, testDenylistBlocksRemoteCodeExecution,
testDenylistBlocksHistoryReplay, testDenylistBlocksCrontabDestruction,
testDenylistBlocksMacOSSystemIntegrity,
testDenylistBlocksContainerMassDestruction,
testDenylistBlocksNetworkDestruction, and testDenylistBlocksLaunchctlDestruction
with calls to that helper to keep coverage but reduce duplication.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Sources/AppDelegate.swift (1)

3712-3717: The non-terminating path still writes the previous cache state.

Line 3716 kicks off a fire-and-forget refresh, but buildSessionSnapshot(...) runs immediately after this block, so non-terminating saves still serialize whatever was already in SessionForegroundProcessCache. If that one-save lag is intentional, I'd make it explicit in the helper/comment or move the async refresh after snapshot construction so the control flow matches the behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 3712 - 3717, The current
fire-and-forget call to refreshForegroundProcessCacheAsync() runs before
buildSessionSnapshot(...), causing non-terminating saves to serialize the
previous SessionForegroundProcessCache; either change the non-terminating path
to call refreshForegroundProcessCacheSync() so the cache is up-to-date before
calling buildSessionSnapshot(...), or move the
refreshForegroundProcessCacheAsync() call to after buildSessionSnapshot(...) and
add a comment explaining the intentional one-save lag, so control flow and
behavior for isTerminatingApp, refreshForegroundProcessCacheSync(),
refreshForegroundProcessCacheAsync(), buildSessionSnapshot(...), and
SessionForegroundProcessCache are consistent and clear.

cmuxTests/SessionPersistenceTests.swift (1)

1096-1322: Consider adding test coverage for the enabled toggle.

The tests thoroughly cover allowlist/denylist pattern matching via isCommandAllowed(_:rawAllowlist:), but there's no test verifying that isCommandAllowed(_:defaults:) returns false when sessionRestoreCommandsEnabled is set to false in UserDefaults.

This gap means the fix from commit ac5665d (respecting the enabled toggle) isn't covered by unit tests.

📝 Suggested test addition

func testDisabledSettingBlocksAllCommands() {
    let defaults = UserDefaults(suiteName: "SessionRestoreCommandSettingsTests")!
    defaults.removePersistentDomain(forName: "SessionRestoreCommandSettingsTests")
    
    // Explicitly disable restore commands
    defaults.set(false, forKey: SessionRestoreCommandSettings.enabledKey)
    
    // Even allowlisted commands should be blocked when disabled
    XCTAssertFalse(SessionRestoreCommandSettings.isCommandAllowed("opencode", defaults: defaults))
    XCTAssertFalse(SessionRestoreCommandSettings.isCommandAllowed("npm run dev", defaults: defaults))
}

func testEnabledByDefaultWhenKeyNotSet() {
    let defaults = UserDefaults(suiteName: "SessionRestoreCommandSettingsTests")!
    defaults.removePersistentDomain(forName: "SessionRestoreCommandSettingsTests")
    
    // Key not set should default to enabled
    XCTAssertTrue(SessionRestoreCommandSettings.isCommandAllowed("opencode", defaults: defaults))
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/SessionPersistenceTests.swift` around lines 1096 - 1322, Add unit
tests to cover the enabled toggle: create tests that set a custom UserDefaults
suite, set SessionRestoreCommandSettings.enabledKey to false and assert
SessionRestoreCommandSettings.isCommandAllowed("opencode", defaults: defaults)
and isCommandAllowed("npm run dev", defaults: defaults) return false; also add a
test where the key is absent (removePersistentDomain) and assert
isCommandAllowed("opencode", defaults: defaults) returns true to ensure
default-enabled behavior. Reference
SessionRestoreCommandSettings.isCommandAllowed(_:defaults:) and
SessionRestoreCommandSettings.enabledKey when adding these tests.

Sources/Workspace.swift (1)

653-691: Consider centralizing restore-command normalization.

Trim + allowlist validation now lives in the restore-selection closure, the post-restore persistence block, and setPanelRestoreCommand. A small helper would keep execute/persist/reattach behavior from drifting the next time the safety rules change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 653 - 691, The restore-command trimming
and allowlist check logic is duplicated across the restore selection closure,
the persistence block, and setPanelRestoreCommand; create a single helper (e.g.,
normalizeAndValidateRestoreCommand(_:)->String?) that trims input, returns nil
for empty strings, and returns the trimmed value only if
SessionRestoreCommandSettings.isCommandAllowed(...) and use it everywhere:
replace the inline trimming/validation in the closure where commandToRestore is
computed, the post-restore persistence block that currently trims/passes to
setPanelRestoreCommand, and update setPanelRestoreCommand to accept
already-normalized input or call the helper internally so all
execute/persist/reattach code paths share the same normalization/validation
behavior.

Sources/GhosttyTerminalView.swift (1)

3158-3164: ⚠️ Potential issue | 🟠 Major

Validate the template initialInput fallback too.

The explicit path is sanitized here, but resolvedInitialInput still forwards baseConfig.initialInput unchanged. Because initial_input is also rehydrated from native surface config in Sources/Workspace.swift:29-31, a newline-bearing template value can bypass this guard and still reach surfaceConfig.initial_input. Please apply the same empty/CRLF normalization to the fallback path, or funnel both paths through one helper.

Also applies to: 3888-3895

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/GhosttyTerminalView.swift` around lines 3158 - 3164, The
resolvedInitialInput fallback path is not normalized so newline-bearing
templates from baseConfig.initialInput or surfaceConfig.initial_input can bypass
the CR/LF check; update resolvedInitialInput to trim whitespace/newlines and
reject empty or CR/LF-containing strings (same logic used where initialInput is
set) or refactor both assignments into a shared sanitizer helper (e.g.,
sanitizeInitialInput(_:)) and call it from places that set initialInput or
resolvedInitialInput, referencing initialInput, resolvedInitialInput,
baseConfig.initialInput and surfaceConfig.initial_input to locate where to apply
the change.

Sources/AppDelegate.swift (1)

3995-4011: Minor: factor out the shared TTY collection helper.

Both methods duplicate the same mainWindowContexts walk and dedupe logic. A tiny helper keeps future filtering changes aligned between the async and sync paths.

♻️ Suggested refactor

+    private func allSessionTerminalTTYNames() -> [String] {
+        Array(Set(mainWindowContexts.values.flatMap { context in
+            context.tabManager.allTerminalTTYNames()
+        }))
+    }
+
     /// Refresh the foreground process cache for all terminal panels (async, non-blocking).
     private func refreshForegroundProcessCacheAsync() {
-        // Dedupe TTY names to avoid redundant ps scans
-        let allTTYNames = Set(mainWindowContexts.values.flatMap { context in
-            context.tabManager.allTerminalTTYNames()
-        })
-        SessionForegroundProcessCache.shared.refresh(ttyNames: Array(allTTYNames))
+        SessionForegroundProcessCache.shared.refresh(ttyNames: allSessionTerminalTTYNames())
     }
 
     /// Refresh the foreground process cache for all terminal panels (blocking, for quit).
     private func refreshForegroundProcessCacheSync() {
-        // Dedupe TTY names to avoid redundant ps scans
-        let allTTYNames = Set(mainWindowContexts.values.flatMap { context in
-            context.tabManager.allTerminalTTYNames()
-        })
-        SessionForegroundProcessCache.shared.refreshSync(ttyNames: Array(allTTYNames))
+        SessionForegroundProcessCache.shared.refreshSync(ttyNames: allSessionTerminalTTYNames())
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 3995 - 4011, Extract the duplicated
walk-and-dedupe logic into a small private helper (e.g. private func
allTerminalTTYNames() -> [String]) that builds the Set from
mainWindowContexts.values.flatMap { $0.tabManager.allTerminalTTYNames() } and
returns an Array of the deduped names; then update
refreshForegroundProcessCacheAsync() to call
SessionForegroundProcessCache.shared.refresh(ttyNames: allTerminalTTYNames())
and refreshForegroundProcessCacheSync() to call
SessionForegroundProcessCache.shared.refreshSync(ttyNames:
allTerminalTTYNames()) so both paths share the same collection logic.

cmuxTests/SessionPersistenceTests.swift (1)

1096-1394: Add one persistence-path regression test.

Everything here exercises SessionRestoreCommandSettings directly, but nothing asserts that a blocked detectedCommand / restoreCommand is actually omitted from the serialized session snapshot. That is the high-value boundary for this feature, and a regression there would still leave this suite green.

Based on learnings: "unallowed (potentially sensitive) commands are never written to the session JSON."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/SessionPersistenceTests.swift` around lines 1096 - 1394, Add a
regression test that verifies blocked commands are never written to the
serialized session snapshot: create a session (using the same test harness in
SessionPersistenceTests.swift), add an entry whose
detectedCommand/restoreCommand would be blocked by
SessionRestoreCommandSettings.isCommandAllowed (e.g., "rm -rf /"), run the
session serialization/path that produces the session JSON via the existing
persistence/serializer code path, and assert the produced JSON/string does not
contain that command and that the corresponding entry is omitted; reference
SessionRestoreCommandSettings.isCommandAllowed and the
detectedCommand/restoreCommand fields to locate the right places to construct
the test.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Sources/Workspace.swift (1)

676-700: ⚠️ Potential issue | 🟠 Major

Use isRemoteBacked to choose the restored backend.

Lines 695-700 still route every terminal restore through newTerminalSurface(), and that helper always derives initialCommand from the workspace-wide remoteTerminalStartupCommand(). In a remote-configured workspace, a locally attached terminal will still be recreated as SSH-backed after restart; the per-panel flag currently only controls whether replay text is sent.

🛠️ Suggested change

-            guard let terminalPanel = newTerminalSurface(
+            let restoredInitialCommand = panelWasRemoteBacked ? remoteTerminalStartupCommand() : nil
+            guard let terminalPanel = newTerminalSurface(
                 inPane: paneId,
                 focus: false,
                 workingDirectory: workingDirectory,
                 startupEnvironment: replayEnvironment,
+                initialCommandOverride: restoredInitialCommand,
                 initialInput: commandToRestore
             ) else {

-    func newTerminalSurface(
+    func newTerminalSurface(
         inPane paneId: PaneID,
         focus: Bool? = nil,
         workingDirectory: String? = nil,
         startupEnvironment: [String: String] = [:],
+        initialCommandOverride: String? = nil,
         initialInput: String? = nil
     ) -> TerminalPanel? {
         let shouldFocusNewTab = focus ?? (bonsplitController.focusedPaneId == paneId)
         let previousFocusedPanelId = focusedPanelId
         let previousHostedView = focusedTerminalPanel?.hostedView

         let inheritedConfig = inheritedTerminalConfig(inPane: paneId)
-        let remoteTerminalStartupCommand = remoteTerminalStartupCommand()
-        // Skip initialInput for SSH (restore text would land in prompts)
-        let safeInitialInput = remoteTerminalStartupCommand != nil ? nil : initialInput
+        let resolvedInitialCommand = initialCommandOverride ?? remoteTerminalStartupCommand()
+        // Skip initialInput for SSH (restore text would land in prompts)
+        let safeInitialInput = resolvedInitialCommand != nil ? nil : initialInput
         let newPanel = TerminalPanel(
             workspaceId: id,
             context: GHOSTTY_SURFACE_CONTEXT_SPLIT,
             configTemplate: inheritedConfig,
             workingDirectory: workingDirectory,
             portOrdinal: portOrdinal,
-            initialCommand: remoteTerminalStartupCommand,
+            initialCommand: resolvedInitialCommand,
             initialInput: safeInitialInput,
             additionalEnvironment: startupEnvironment
         )

Based on learnings: createPanel(from:inPane:) should use the per-panel isRemoteBacked flag so local terminals attached to remote-configured workspaces restore as local panels.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 676 - 700, The restore path currently
ignores per-panel backend and always uses workspace-level
remoteTerminalStartupCommand(), causing locally attached terminals to be
recreated as SSH-backed; update createPanel(from:inPane:) (the code that calls
newTerminalSurface(inPane:focus:workingDirectory:startupEnvironment:initialInput:))
to check the panel's snapshot.terminal?.isRemoteBacked (or isRemoteBacked) and
pass the correct backend decision into newTerminalSurface so that when
isRemoteBacked == false you do not derive startup backend from
remoteTerminalStartupCommand() and instead restore as a local terminal while
still preserving initialInput (commandToRestore) behavior and allowlist
validation via SessionRestoreCommandSettings.

Sources/AppDelegate.swift (1)

2863-2885: ⚠️ Potential issue | 🟠 Major

The precautionary async save can still clobber the final termination snapshot.

This save is still queued before the quit path is resolved. If shutdown proceeds, applicationWillTerminate(_:) does the fresh synchronous save, but the older async write can still finish afterward and overwrite it with stale foreground-process data. Guard async persistence once termination starts, or serialize both write paths through the same queue.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 2863 - 2885, The async precautionary
save called in applicationShouldTerminate(_:) via
saveSessionSnapshot(includeScrollback:removeWhenEmpty:) can complete after
applicationWillTerminate(_:) performs a synchronous final save and thus
overwrite it with stale data; fix by preventing that race: when entering the
quit path set isTerminatingApp (or check that flag) before scheduling the async
save and skip or cancel the async write if termination has started, or instead
funnel both async and sync saves through a single serial persistence queue used
by saveSessionSnapshot and applicationWillTerminate so the final synchronous
save always runs after any pending async work; update
applicationShouldTerminate(_:), saveSessionSnapshot(...), and
applicationWillTerminate(_:) to use the shared queue or honor isTerminatingApp
to avoid the clobber.

cmuxTests/SessionPersistenceTests.swift (1)

1627-1668: ⚠️ Potential issue | 🟠 Major

Denylist tests still have allowlist short-circuit cases.

On Line 1643 (AWS_ACCESS_KEY_ID=... aws s3 ls) and Line 1660-1662 (sshpass ...), commands are not matched by the provided allowlists, so failures can come from allowlist rejection rather than denylist enforcement. That weakens these security regressions tests.

✅ Suggested test hardening

     func testDenylistBlocksSensitiveCredentials() {
-        // Commands with tokens/credentials anywhere should be blocked
-        let allowlist = "opencode *\nnpm *\naws *\nmycli *\npsql *\ngit *"
-        assertAllBlocked([
+        let commands = [
             // API keys and tokens
             "opencode --api-key=test-key-here",
             "opencode --token=test-token",
             "npm run dev --access-token=test-token",
             "mycli --bearer=test-bearer",
             "mycli --secret=test-secret",
             // Passwords
             "mycli --password=test-pass",
             "psql --passwd=test-pass",
             // AWS credentials
             "aws --aws-access-key-id=AKIAIOSFODNN7EXAMPLE",
             "aws --aws-secret-access-key=test-secret",
             "AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE aws s3 ls",
             // Database connection strings
             "mycli mongodb://user:pass@host/db",
             "mycli postgresql://user:pass@host/db",
             // SSH keys
             "mycli --private-key=/path/to/key",
             // Sensitive file access
             "mycli cat .ssh/id_rsa",
             "mycli cat .aws/credentials",
             "mycli cat .kube/config",
-        ], allowlist: allowlist)
+        ]
+        let allowlist = commands.joined(separator: "\n")
+        assertAllBlocked(commands, allowlist: allowlist)
     }

     func testDenylistBlocksSSHWithCredentials() {
-        let allowlist = "ssh *\nmosh *"
-        assertAllBlocked([
+        let commands = [
             // sshpass password wrapper
             "sshpass -p secret ssh user@host",
             "sshpass -f /path/to/passfile ssh user@host",
             // user:pass@host syntax
             "ssh user:password@host",
             "mosh user:secret@server.com",
             // Default key paths
             "ssh -i ~/.ssh/id_rsa user@host",
             "ssh -i .ssh/id_ed25519 user@host",
-        ], allowlist: allowlist)
+        ]
+        let allowlist = commands.joined(separator: "\n")
+        assertAllBlocked(commands, allowlist: allowlist)
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/SessionPersistenceTests.swift` around lines 1627 - 1668, In
testDenylistBlocksSensitiveCredentials and testDenylistBlocksSSHWithCredentials,
some test cases (the "AWS_ACCESS_KEY_ID=... aws s3 ls" string in
testDenylistBlocksSensitiveCredentials and the "sshpass ..." / "ssh
user:password@host" entries in testDenylistBlocksSSHWithCredentials) can be
short‑circuited by the allowlist; update the tests so the denylist is actually
exercised: either remove or tighten the allowlist entries ("aws *", "ssh *",
"mosh *") used in these two tests, or move those specific command strings into
separate test cases whose allowlist intentionally does not match them; modify
the allowlist strings referenced in these tests (the local variables allowlist)
or split the test arrays in the functions testDenylistBlocksSensitiveCredentials
and testDenylistBlocksSSHWithCredentials so the denylist logic is tested
directly for the listed AWS and sshpass/username:password cases.

Sources/Workspace.swift (2)

8011-8019: ⚠️ Potential issue | 🟡 Minor

Don't clear a moved remote tab's carried TTY here.

attachDetachedSurface(...) rehydrates detached.ttyName before remote tracking is re-adopted. This unconditional clear removes it again, so the panel falls out of TTY-scoped remote port scanning after the next syncRemotePortScanTTYs() until the shell reports a new TTY. Preserve a non-empty existing TTY here, or reapply the detached TTY after trackRemoteTerminalSurface().

Based on learnings, WorkspaceRemoteSessionController.updateRemotePortScanTTYsLocked(_:) must drop remoteScannedPortsByPanel entries when a panel’s TTY changes, so clearing a carried TTY here causes moved remote panels to fall out of tracking on the next sync.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 8011 - 8019, The code in
trackRemoteTerminalSurface(_) unconditionally clears surfaceTTYNames for the
panelId (surfaceTTYNames.removeValue(forKey: panelId)), which wipes out a
detached.ttyName rehydrated earlier in attachDetachedSurface(...) and causes the
panel to drop out of TTY-scoped remote port scanning; change this so you do not
remove an existing non-empty TTY: either skip the removeValue call when
surfaceTTYNames[panelId] is non-empty or reapply detached.ttyName immediately
after tracking is adopted (e.g. call applyPendingRemoteSurfaceTTYIfNeeded(to:)
or restore the detached TTY before/after activeRemoteTerminalSurfaceIds.insert),
and ensure updateRemotePortScanTTYsLocked(_:) will see the TTY change so
remoteScannedPortsByPanel entries are updated correctly.

---

662-680: ⚠️ Potential issue | 🟠 Major

Per-panel remote/local identity still isn't applied during restore.

panelWasRemoteBacked only suppresses replay metadata. The actual constructor path still picks initialCommand from the workspace-wide remoteTerminalStartupCommand(), so a restored workspace cannot faithfully recreate a mix of SSH-backed terminals and detached-in local terminals. That also lets Lines 678-680 repopulate a local restore override onto a panel that may have just been recreated as remote. Thread the per-panel backend flag into terminal creation instead of only using it to gate initialInput and panelRestoreCommands.

Also applies to: 8695-8705

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 662 - 680, The restore path currently
only uses panelWasRemoteBacked to gate initialInput but still lets the
constructor pick a workspace-wide remoteTerminalStartupCommand(), so add
per-panel backend information into terminal creation: modify the call to
newTerminalSurface(inPane:focus:workingDirectory:startupEnvironment:initialInput:)
to accept and pass a per-panel isRemoteBacked flag (derived from
snapshot.terminal?.isRemoteBacked as stored in panelWasRemoteBacked) and update
newTerminalSurface/terminal construction logic to prefer per-panel behavior for
choosing initialCommand (falling back to workspace
remoteTerminalStartupCommand() only when per-panel isRemoteBacked is true or
unset). Also change the post-creation restore-override logic (the
setPanelRestoreCommand(panelId:command:) conditional) to check the actual
terminalPanel.isRemoteBacked (not just panelWasRemoteBacked) so you don't apply
local restore overrides to panels created as remote; apply the same change in
the other affected block noted (around lines 8695-8705).

Sources/AppDelegate.swift (1)

4026-4042: Extract the shared TTY-name collection.

Both methods duplicate the same collection/dedupe pipeline, so future filtering or normalization changes can drift between the async and sync paths.

♻️ Small cleanup

+    private func foregroundProcessCacheTTYNames() -> [String] {
+        Array(Set(mainWindowContexts.values.flatMap { context in
+            context.tabManager.allTerminalTTYNames()
+        }))
+    }
+
     /// Refresh the foreground process cache for all terminal panels (async, non-blocking).
     private func refreshForegroundProcessCacheAsync() {
-        // Dedupe TTY names to avoid redundant ps scans
-        let allTTYNames = Set(mainWindowContexts.values.flatMap { context in
-            context.tabManager.allTerminalTTYNames()
-        })
-        SessionForegroundProcessCache.shared.refresh(ttyNames: Array(allTTYNames))
+        SessionForegroundProcessCache.shared.refresh(ttyNames: foregroundProcessCacheTTYNames())
     }
 
     /// Refresh the foreground process cache for all terminal panels (blocking, for quit).
     private func refreshForegroundProcessCacheSync() {
-        // Dedupe TTY names to avoid redundant ps scans
-        let allTTYNames = Set(mainWindowContexts.values.flatMap { context in
-            context.tabManager.allTerminalTTYNames()
-        })
-        SessionForegroundProcessCache.shared.refreshSync(ttyNames: Array(allTTYNames))
+        SessionForegroundProcessCache.shared.refreshSync(ttyNames: foregroundProcessCacheTTYNames())
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 4026 - 4042, Extract the duplicated
TTY-name collection into a single helper (e.g., a private method like
collectAllTTYNames() or allTTYNames()) that performs the Set dedupe over
mainWindowContexts.values.flatMap { context in
context.tabManager.allTerminalTTYNames() } and returns an Array of TTY names;
then replace the duplicated code in refreshForegroundProcessCacheAsync() and
refreshForegroundProcessCacheSync() so they each call the new helper and pass
its result to SessionForegroundProcessCache.shared.refresh(...) and
refreshSync(...), respectively. Ensure the helper is private and used only by
these two methods so future normalization/filtering changes stay in one place.

cmuxTests/SessionPersistenceTests.swift (1)

1203-1233: Isolate UserDefaults suites per test to avoid shared-state coupling.

Using one suite name ("SessionRestoreCommandSettingsTests") across tests can make behavior order-dependent under parallel runs. Prefer a per-test suite name plus cleanup via defer.

Refactor sketch

+    private func makeIsolatedDefaults(file: StaticString = `#filePath`, line: UInt = `#line`) -> (UserDefaults, String) {
+        let suite = "SessionRestoreCommandSettingsTests.\(UUID().uuidString)"
+        let defaults = UserDefaults(suiteName: suite)!
+        defaults.removePersistentDomain(forName: suite)
+        return (defaults, suite)
+    }

     func testDisabledSettingBlocksAllCommands() {
-        let defaults = UserDefaults(suiteName: "SessionRestoreCommandSettingsTests")!
-        defaults.removePersistentDomain(forName: "SessionRestoreCommandSettingsTests")
+        let (defaults, suite) = makeIsolatedDefaults()
+        defer { defaults.removePersistentDomain(forName: suite) }
         defaults.set(false, forKey: SessionRestoreCommandSettings.enabledKey)
         ...
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/SessionPersistenceTests.swift` around lines 1203 - 1233, Tests
share the same UserDefaults suite "SessionRestoreCommandSettingsTests", causing
inter-test coupling; update each test to create a unique suite (e.g., append
UUID or test name when calling UserDefaults(suiteName:)), use that unique
defaults when setting/getting SessionRestoreCommandSettings.enabledKey and
SessionRestoreCommandSettings.allowlistKey, and ensure cleanup by removing the
persistent domain in a defer block (defaults.removePersistentDomain(forName:
...)) so SessionRestoreCommandSettings.isCommandAllowed uses an isolated
UserDefaults instance per test.

Sources/SessionPersistence.swift (1)

1141-1147: Minor: Duplicated normalizeTTYName implementation.

The TTY normalization logic is duplicated between SessionForegroundProcessCache.normalizeTTYName and SessionForegroundProcessDetector.normalizeTTYName. Consider extracting to a shared utility, though the current duplication is acceptable for keeping the classes independent.

Also applies to: 1073-1079

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/SessionPersistence.swift` around lines 1141 - 1147, Duplicate TTY
normalization exists in SessionForegroundProcessCache.normalizeTTYName and
SessionForegroundProcessDetector.normalizeTTYName; extract the
trimming-and-/dev/ stripping logic into a single shared helper (for example a
free function normalizeTTYName(_: String) or a String extension method) and
replace both class-private implementations to call that shared helper so the
logic is centralized and the duplicate methods removed.